Skip to content

Make TypeId const comparable #142789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Make TypeId const comparable #142789

wants to merge 8 commits into from
+316 −147

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 20, 2025 •
edited
Loading

This should unblock stabilizing const TypeId::of and allow us to progress into any possible future we want to take TypeId to.

To achieve that TypeId now contains 16 / size_of<usize>() pointers which each are actually just size_of<usize>() bytes of the stable hash. At compile-time these pointers cannot be dereferenced or otherwise inspected (at present doing so might ICE the compiler). Preventing inspection of this data allows us to refactor TypeId to any other scheme in the future without breaking anyone who was tempted to transmute TypeId to obtain the hash at compile-time.

cc @eddyb for their previous work on #95845 (which we still can do in the future if we want to get rid of the hash as the final thing that declares two TypeIds as equal).

r? @RalfJung

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 20, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

Sign in to view
Dante-Broggi
Dante-Broggi reviewed Jun 21, 2025
View reviewed changes
Noratrieb
Noratrieb reviewed Jun 21, 2025
View reviewed changes
@oli-obk oli-obk force-pushed the constable-type-id branch from 3cddd21 to 1fd7b66 Compare June 21, 2025 10:20
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member

It will be a while until I have the capacity to review a PR of this scale.

Meanwhile, could you say a bit more about the architecture of the change? It seems you want for the "new kind of allocation" approach, but it's not clear from the PR description how exactly that shows up in TypeId.

Also, I am definitely not comfortable landing this by myself, I can only review the const-eval parts. Changing the representation of TypeId has ramifications well beyond that that I do not feel qualified to evaluate -- I think an MCP would be justified.

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2025

Well, I got private feedback yesterday that instead of encoding a 16 byte value as an 8 byte pointer to the 16 byte value and an 8 byte hash, I should just do the thing where we split up type id internally into pointer sized chunks and codegen will make a hash out of it again.

TLDR: no changes to runtime type id anymore in the latest revision of this PR. Only compile-time type id is now a bit funny

@oli-obk oli-obk mentioned this pull request Jun 21, 2025
Merged
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2025

It will be a while until I have the capacity to review a PR of this scale.

I'm splitting unrelated parts out, so the high level feedback is already useful and I'll look for libs and codegen ppl to review the appropriate parts

@rust-log-analyzer

This comment has been minimized.

Sign in to view
antoyo
antoyo reviewed Jun 21, 2025
View reviewed changes
bjorn3
bjorn3 reviewed Jun 21, 2025
View reviewed changes
jdonszelmann added a commit to jdonszelmann/rust that referenced this pull request Jun 23, 2025
@jdonszelmann
Rollup merge of rust-lang#142822 - oli-obk:const-partial-eq, r=fee1-dead
443fd52 
Make `PartialEq` a `const_trait`

r? `@fee1-dead` or `@compiler-errors`

something generally useful but also required for rust-lang#142789
jdonszelmann added a commit to jdonszelmann/rust that referenced this pull request Jun 23, 2025
@jdonszelmann
Rollup merge of rust-lang#142822 - oli-obk:const-partial-eq, r=fee1-dead
c8df66f 
Make `PartialEq` a `const_trait`

r? ``@fee1-dead`` or ``@compiler-errors``

something generally useful but also required for rust-lang#142789
rust-timer added a commit that referenced this pull request Jun 23, 2025
@rust-timer
Unrolled build for #142822
719f8af 
Rollup merge of #142822 - oli-obk:const-partial-eq, r=fee1-dead

Make `PartialEq` a `const_trait`

r? ``@fee1-dead`` or ``@compiler-errors``

something generally useful but also required for #142789
@bors
Copy link
Collaborator

bors commented Jun 23, 2025

☔ The latest upstream changes (presumably #142906) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the constable-type-id branch 2 times, most recently from b8a7a10 to 1c47a64 Compare June 24, 2025 09:25
bjorn3
bjorn3 reviewed Jun 24, 2025
View reviewed changes
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the constable-type-id branch from 1c47a64 to bcb4aa2 Compare June 24, 2025 13:02
@rust-log-analyzer

This comment has been minimized.

Sign in to view
bjorn3
bjorn3 reviewed Jun 24, 2025
View reviewed changes
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2025

I'm honestly still not super happy with that proposal, but you seem set on doing something along these lines and this is a compromise that I think I can live with, if it doesn't turn out to have unexpected problems. This will still require a bunch of special hacks to make these "allocations" have a known-0 base address, but I can't think of a better way (making the base address be the hash is not better, IMO).

If we're exposing any bytes to const eval, we can just expose everything and revisit adding provenance when T-lang approves a reflection experiment. I'll create a separate PR for just making the PartialEq impl const and propose moving TypeId to stabilization along with stabilizing the const trait impl without stabilizing const traits in general

@RalfJung
Copy link
Member

If we're exposing any bytes to const eval, we can just expose everything

Having provenance on some bytes still means we'd appropriately error out if someone just transmutes the entire thing to u128 in const-eval, or something like that. We'd only be truly exposing the "tail", i.e., everything after the first ptr-sized chunk.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 27, 2025 •
edited
Loading

I meant more in the sense of "Ralf is happy with this approach" XD

I would prefer to prevent such transmutes failing by having provenance $somewhere, so I'll stop talking myself out of it now 😆

@oli-obk oli-obk force-pushed the constable-type-id branch from afa74de to 6fea6c3 Compare June 27, 2025 13:58
@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the constable-type-id branch from 6fea6c3 to 2e8cece Compare June 27, 2025 14:24
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jun 27, 2025

☔ The latest upstream changes (presumably #143091) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 28, 2025
@oli-obk oli-obk force-pushed the constable-type-id branch from 2e8cece to aa4c156 Compare June 30, 2025 13:19
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the constable-type-id branch from aa4c156 to 8a42250 Compare June 30, 2025 15:34
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 30, 2025 •
edited
Loading

Having provenance on some bytes still means we'd appropriately error out if someone just transmutes the entire thing to u128 in const-eval, or something like that. We'd only be truly exposing the "tail", i.e., everything after the first ptr-sized chunk.

I implemented this in 8a42250

It made things a lot cleaner and simpler

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fasterthanlime fasterthanlime mentioned this pull request Jul 1, 2025
Closed
@oli-obk oli-obk force-pushed the constable-type-id branch from 8a42250 to 82aa2a9 Compare July 1, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoyo antoyo antoyo left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@Dante-Broggi Dante-Broggi Dante-Broggi left review comments

@Noratrieb Noratrieb Noratrieb left review comments

Assignees

@RalfJung RalfJung

Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@oli-obk @rustbot @rust-log-analyzer @RalfJung @bors @antoyo @bjorn3 @Dante-Broggi @Noratrieb